Conversation
1c2a0b8 to
ce5325f
Compare
de2593d to
0abae51
Compare
codex-rs/analytics/src/reducer.rs
Outdated
| out: &mut Vec<TrackEventRequest>, | ||
| ) { | ||
| let Some(RequestState::TurnSteer(pending_request)) = | ||
| self.requests.remove(&(connection_id, request_id)) |
There was a problem hiding this comment.
do we want to do this for TurnStart too? in case a JSON RPC error gets thrown for turn/start
codex-rs/analytics/src/reducer.rs
Outdated
| if error.message.starts_with("expected active turn id ") { | ||
| return Some(TurnSteerRejectionReason::ExpectedTurnMismatch); | ||
| } | ||
| if error.message == "input must not be empty" { |
There was a problem hiding this comment.
hmm, should we figure out a less brittle way to do this? maybe define shared constants for the strings?
codex-rs/analytics/src/reducer.rs
Outdated
| .get("codexErrorInfo")? | ||
| .get("activeTurnNotSteerable")? | ||
| .get("turnKind")? | ||
| .as_str()?; |
There was a problem hiding this comment.
hmm yeah... ideally there's a way to match on CodexErrorInfo instead of JSONRPCErrorError because then you'll have stronger type guarantees
There was a problem hiding this comment.
or maybe you can just cast data.get("codexErrorInfo") back to CodexErrorInfo and then pull out turnKind
| ErrorResponse { | ||
| connection_id: u64, | ||
| request_id: RequestId, | ||
| error: JSONRPCErrorError, |
There was a problem hiding this comment.
what do you think of adding something well-typed here actually, so we don't have to do string parsing on JSONRPCErrorError?
maybe:
ErrorResponse {
connection_id: u64,
request_id: RequestId,
error: JSONRPCErrorError,
type: Option<JsonRpcErrorType>,
}
where type is something like:
pub enum JsonRpcErrorType {
Turn(TurnRequestError),
Input(InputError),
}
pub enum TurnRequestError {
NoActiveTurn,
ExpectedTurnMismatch,
ActiveTurnNotSteerable { turn_kind: NonSteerableTurnKind },
}
pub enum InputError {
Empty,
TooLarge { max_chars: usize, actual_chars: usize },
}
something like that? just brainstorming
owenlin0
left a comment
There was a problem hiding this comment.
looks like there's a clippy error:
--> analytics/src/analytics_client_tests.rs:306:1
|
153 | fn sample_runtime_metadata() -> CodexRuntimeMetadata {
| ---------------------------------------------------- previous definition of the value sample_runtime_metadata here
...
306 | fn sample_runtime_metadata() -> CodexRuntimeMetadata {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ sample_runtime_metadata redefined here
codex-rs/analytics/src/reducer.rs
Outdated
| .count() | ||
| } | ||
|
|
||
| fn now_unix_seconds() -> u64 { |
There was a problem hiding this comment.
nit: i've seen this function defined in a bunch of places at this point, should we centralize it somewhere? can do in a followup
| message: "expectedTurnId must not be empty".to_string(), | ||
| data: None, | ||
| }; | ||
| self.track_error_response(&request_id, &error, /*error_type*/ None); |
There was a problem hiding this comment.
hmm let's revert this? i don't think it's worth tracking this one
| .as_secs() | ||
| } | ||
|
|
||
| fn rejection_reason_from_error_type( |
There was a problem hiding this comment.
this is a lot of boilerplate, any way we can eliminate this?
owenlin0
left a comment
There was a problem hiding this comment.
minor comments but preapproving
Stack created with Sapling. Best reviewed with ReviewStack.